Skip to content

r.sim: disable broken nblock usage#7358

Merged
petrasovaa merged 3 commits into
OSGeo:mainfrom
petrasovaa:r.sim-nblocks
May 21, 2026
Merged

r.sim: disable broken nblock usage#7358
petrasovaa merged 3 commits into
OSGeo:mainfrom
petrasovaa:r.sim-nblocks

Conversation

@petrasovaa
Copy link
Copy Markdown
Contributor

TL;DR

Disabled the broken nblock >1 auto-split in r.sim.water/r.sim.sediment by forcing nblock=1. Kept the variables so that nblocks functionality can be enabled in follow up PRs.

Fixed the err formula to the original fortran's Monte Carlo stddev (fixes #4799). As a result error map is now 0 and will become more useful once nblocks will be enabled to be > 1.

Background

nblock originated in the Fortran ancestor as a memory workaround for running more walkers than the static buffer held; the C port inherited the structure but eventually mangled the per-block walker
accounting (rwalk was never actually divided), silently producing biased depth/discharge for users beyond the MAXW cap.

Summary of changes

  • Disable the broken auto-split path in r.sim.water / r.sim.sediment: force nblock = 1, drop the MAXW static cap, and remove the obsolete "ERROR: nwalk > maxw" doc reference.
  • Repair the err output to the Fortran's Monte Carlo standard-deviation formula (sqrt(|E[(gama·conn)²]/nblock − gama²|)); previously the C port had a wrong exponent and a missing
    post-loop step that produced a quantity with no clean meaning (fixes [Bug] r.sim.water: error output is not error but sum of depths? #4799). With nblock = 1 today it's zero everywhere; it becomes meaningful when nblock > 1 is reintroduced.
  • Introduce conn = nblock/iblock to gama in output_data for depth, discharge, conc, and flux outputs, matching the Fortran's intermediate-snapshot extrapolation. With nblock = 1 conn = 1.0
    and outputs are unchanged.
  • Preserve nblock, conn, and the iblock loop as scaffolding for sequential-replica reintroduction (Fortran-faithful, with working variance estimator) followed by thread-parallel replicas.

Behavior changes

  • nblock = 1 users (the only path that actually worked): depth, discharge, conc, flux, erdep unchanged. err output changes from pow(gama_final, 3/5) (which had no clean physical meaning)
    to all-zeros (correct stddev for one sample).
  • Users beyond the old MAXW cap: previously got silently biased outputs from the broken auto-split

Not in this PR

  • Reintroducing nblock > 1 (sequential-replica execution) — to follow.
  • Thread-parallel replica execution and the OpenMP race in the per-walker loop — separate follow-up.

@petrasovaa petrasovaa requested a review from wenzeslaus May 1, 2026 18:25
@github-actions github-actions Bot added raster Related to raster data processing C Related code is in C HTML Related code is in HTML module docs markdown Related to markdown, markdown files labels May 1, 2026
metzm
metzm previously approved these changes May 2, 2026
Copy link
Copy Markdown
Contributor

@metzm metzm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me, thanks!

Looking forward to a proposal enabling nblocks > 1.

Copy link
Copy Markdown
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The accumulation as total (sum) as opposed to average is not my first expectation when replica term is used, but with explanation it makes sense as result being an aggregate of multiple replicas (just not the average aggregate). The extrapolated total makes sense in that context although this will probably change with parallel. I'm not sure about the error computation (I did not dive into it).

Considering nblocks is 1 and parallelization needing more rewrite, the current PR is good and makes things consistent within the code. (The sync to original Fortran makes sense too; the code here drifted based on the nblock issue.)

Note: "...produced biased depth..." sounds like this belongs to Errata.

@petrasovaa petrasovaa added the errata Include in errata (fixes wrongly computed results) label May 21, 2026
@petrasovaa petrasovaa merged commit a39b881 into OSGeo:main May 21, 2026
26 checks passed
@petrasovaa petrasovaa deleted the r.sim-nblocks branch May 21, 2026 20:23
@github-actions github-actions Bot added this to the 8.6.0 milestone May 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C Related code is in C docs errata Include in errata (fixes wrongly computed results) HTML Related code is in HTML markdown Related to markdown, markdown files module raster Related to raster data processing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] r.sim.water: error output is not error but sum of depths?

3 participants